Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default session data #501

Merged
merged 3 commits into from
May 10, 2018
Merged

Add default session data #501

merged 3 commits into from
May 10, 2018

Conversation

joelanman
Copy link
Contributor

By @mikeshawdev

If provided, the app will try to merge data from a defaults file with data from the session. Defaults will only apply if there isn't a corresponding value already in the session, so data can be overridden within the prototype if necessary.

@fofr
Copy link
Contributor

fofr commented May 9, 2018

Thanks @mikeshawdev, I've been doing something very similar but in a more hacky way. This is very helpful.

@edwardhorsford
Copy link
Contributor

What about naming the file session-data-defaults.js?

I'd like to separately add a file for storing global data, and intend to call it global-data.js - this would make them both consistent.

start.js Outdated
@@ -17,6 +17,21 @@ if (!envExists) {
.pipe(fs.createWriteStream(path.join(__dirname, '/.env')))
}

// Create template session data defaults file if it doesn't exist
const sessionDefaultsDirectory = path.join(__dirname, '/app/data')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be named defaultDataDirectory - so that in the future if we reference files in this directory we can reuse it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for this idea, I think that would be more suitable

start.js Outdated
const sessionDefaultsExists = fs.existsSync(sessionDefaultsFile)

if (!sessionDefaultsExists) {
console.log('Creating template session data defaults file')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we creating a template file, or just the defaults file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should strip "template" from the sentence as we're creating from a template, not creating a template

start.js Outdated

if (!sessionDefaultsExists) {
console.log('Creating template session data defaults file')
if (!fs.existsSync(sessionDefaultsDirectory)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will be adding a separate global-data.js file also in this directory. I wonder if we can check for the directory separately than checking for the session defaults file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should refactor in that PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can refactor this later.

@joelanman joelanman force-pushed the mike-shaw-session-defaults branch 2 times, most recently from ff6ecf2 to 8618f5b Compare May 10, 2018 14:20
@edwardhorsford
Copy link
Contributor

Looks good to me 👍

Mike Shaw and others added 3 commits May 10, 2018 15:39
If provided, the app will try to merge data from a defaults file with data from the session. Defaults will only apply if there isn't a corresponding value already in the session, so data can be overridden within the prototype if necessary.
@joelanman joelanman merged commit 4d2cb9f into master May 10, 2018
@joelanman joelanman deleted the mike-shaw-session-defaults branch May 10, 2018 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants